Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add hints for schema config expr #1589

Closed
wants to merge 7 commits into from

Conversation

shruti2522
Copy link
Contributor

@shruti2522 shruti2522 commented Aug 21, 2024

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

2. What is the scope of this PR (e.g. component or file name):

kclvm/sema/src/advanced_resolver/node.rs

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Other

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

  • N
  • Y

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

  • Unit test
  • Integration test
  • Benchmark (add benchmark stats below)
  • Manual test (add detailed scripts or steps below)
  • Other

@Peefy
Copy link
Contributor

Peefy commented Aug 21, 2024

Hello @shruti2522

What I mean in #1575 is to add hints to schema arguments, just like function parameters, rather than schema config expr.

@shruti2522
Copy link
Contributor Author

How to check for the scope of config expr here? @He1pa @Peefy

@shruti2522
Copy link
Contributor Author

shruti2522 commented Aug 21, 2024

Hello @shruti2522

What I mean in #1575 is to add hints to schema arguments, just like function parameters, rather than schema config expr.

Okay. I was initially working on adding hints for this, so I misunterstood. Will add hints for schema arguments.

From what I understand it is supposed to be like this? @Peefy @He1pa

schema Name:
    name: str
n: Name = [Name]{  // provide hint of anonymous schema name here
    name: “”
}

@Peefy
Copy link
Contributor

Peefy commented Aug 21, 2024

@shruti2522 Examples of schema arguments

schema Person[age: int, name: str]:
    a = age
    n = name

p = Person([age: ]1, [name: ]"Alice") # Add hints here

@Peefy
Copy link
Contributor

Peefy commented Aug 21, 2024

Of course, adding hints to the schema config expr is also possible. You can open a new issue and associate it with it.

@He1pa
Copy link
Contributor

He1pa commented Aug 22, 2024

Hello @shruti2522
What I mean in #1575 is to add hints to schema arguments, just like function parameters, rather than schema config expr.

Okay. I was initially working on adding hints for this, so I misunterstood. Will add hints for schema arguments.

From what I understand it is supposed to be like this? @Peefy @He1pa

schema Name:
    name: str
n: Name = [Name]{  // provide hint of anonymous schema name here
    name: “”
}

These are two features. Do not associate this PR with #1575, but #1244.

Signed-off-by: shruti2522 <[email protected]>
Signed-off-by: shruti2522 <[email protected]>
kclvm/sema/src/advanced_resolver/node.rs Outdated Show resolved Hide resolved
kclvm/sema/src/advanced_resolver/node.rs Outdated Show resolved Hide resolved
kclvm/sema/src/advanced_resolver/node.rs Outdated Show resolved Hide resolved
kclvm/sema/src/advanced_resolver/node.rs Outdated Show resolved Hide resolved
@shruti2522 shruti2522 marked this pull request as ready for review August 27, 2024 13:06
@shruti2522 shruti2522 changed the title [WIP] feat: add hints for schema config expr feat: add hints for schema config expr Aug 27, 2024
@shruti2522
Copy link
Contributor Author

Screenshot from 2024-08-27 19-24-26

currently, hints are also added to schema config where schema name is alraedy defined, for example,

Screenshot from 2024-08-27 19-23-20

Since config_expr doesn't have a schema name field associated with it, how can we add a check for the name declaration here? Should I do a schema_expr check here or maybe check for identifier expr.

cc @He1pa @Peefy

@Peefy
Copy link
Contributor

Peefy commented Aug 28, 2024

Since config_expr doesn't have a schema name field associated with it, how can we add a check for the name declaration here? Should I do a schema_expr check here or maybe check for identifier expr.

cc @He1pa

Signed-off-by: shruti2522 <[email protected]>

schema name hint

Signed-off-by: shruti2522 <[email protected]>

set sema type

Signed-off-by: shruti2522 <[email protected]>

add expr name

Signed-off-by: shruti2522 <[email protected]>

revert change in fib.k

Signed-off-by: shruti2522 <[email protected]>

conditional hint

Signed-off-by: shruti2522 <[email protected]>

add tests

Signed-off-by: shruti2522 <[email protected]>

add config_entry

Signed-off-by: shruti2522 <[email protected]>

add hints for type assign

Signed-off-by: shruti2522 <[email protected]>

add hint

Signed-off-by: shruti2522 <[email protected]>
@shruti2522
Copy link
Contributor Author

hints for anonymous schema expr:

Screenshot from 2024-08-28 15-04-57

no hints otherwise:

Screenshot from 2024-08-28 15-05-22

PTAL @Peefy @He1pa

@Peefy
Copy link
Contributor

Peefy commented Aug 28, 2024

hints for anonymous schema expr:

Screenshot from 2024-08-28 15-04-57

no hints otherwise:

Screenshot from 2024-08-28 15-05-22

PTAL @Peefy @He1pa

We need to remove : in Name :

Signed-off-by: shruti2522 <[email protected]>
@coveralls
Copy link
Collaborator

coveralls commented Aug 28, 2024

Pull Request Test Coverage Report for Build 10597220055

Details

  • 60 of 61 (98.36%) changed or added relevant lines in 2 files are covered.
  • 5 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.03%) to 70.619%

Changes Missing Coverage Covered Lines Changed/Added Lines %
kclvm/sema/src/advanced_resolver/node.rs 53 54 98.15%
Files with Coverage Reduction New Missed Lines %
kclvm/sema/src/advanced_resolver/node.rs 1 83.6%
kclvm/tools/src/LSP/src/tests.rs 1 98.54%
kclvm/sema/src/core/symbol.rs 3 52.69%
Totals Coverage Status
Change from base Build 10580105310: 0.03%
Covered Lines: 51054
Relevant Lines: 72295

💛 - Coveralls

Signed-off-by: shruti2522 <[email protected]>
column: start.column.map(|c| if c >= 1 { c - 1 } else { 0 }),
};

let with_hint = if let Some(stmt) = self.ctx.program.pos_to_stmt(&pre_pos) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The complexity of this function is a bit high, and other methods can be used to determine it.

Besides, why add hint only for the assign_stmt

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess hints have to applied only for anonymous schema configs, like

schema Name:
    name: str

n: Name = {
    name = "kcl"
}

that's why I used assign_stmt to determine if type annotations exist for the same, if yes then hints are applied, otherwise not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

schema Name:
    name: str
    config: Config

schema Config:
    count: int

n: Name = {
    name = "foo"
    config: {
        count: 1
    }
}

The config in the Name schema also needs hints e.g.

schema Name:
    name: str
    config: Config

schema Config:
    count: int

n: Name = [Name ] { # Name schema hints
    name = "foo"
    config: [Config ] { # Config schema hints
        count: 1
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot from 2024-08-28 18-21-55

hints are being applied for the config too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this code, an assign stmt with the type annotation and the right value is a schema expr.

schema Name:
    name: str
    config: Config

schema Config:
    count: int

n Name = Name { # Name schema hints
    name = "foo"
    config: [Config ] { # Config schema hints
        count: 1
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot from 2024-08-28 18-35-23

I guess we need to add check for schema_expr too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is that there are many such grammars, not just checking schema_exp or assign_stmt, but there needs to be a more effective way.

@Peefy
Copy link
Contributor

Peefy commented Sep 3, 2024

Hello @shruti2522

Have you been working this issue?

@shruti2522
Copy link
Contributor Author

Hello @shruti2522

Have you been working this issue?

Yes @Peefy

@Peefy
Copy link
Contributor

Peefy commented Sep 12, 2024

Hello @shruti2522

Have you been working this issue and PR? I will close this PR until next week.

@Peefy Peefy closed this Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LFX] [Track] LSP inlayhint and hover content content optimization
4 participants